Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use mudder to rank form elements and related pub values #996

Merged
merged 48 commits into from
Mar 5, 2025

Conversation

kalilsn
Copy link
Member

@kalilsn kalilsn commented Feb 25, 2025

Issue(s) Resolved

#963

High-level Explanation of PR

This PR:

  • Adds rank column to both pub_values and form_elements tables
  • Migrates existing form_elements and pub_values to have initial ranks
  • Updates the FormBuilder to generate ranks as elements are created and moved
  • Updates form selection query to order elements by rank
  • Updates pub values queries to order values by rank
  • Fixes a few other bugs in the form builder that I encountered during testing
    • Label changes are reflected in the form builder now
    • We now set a default config.label or config.relationshipConfig.label instead of form_elements.label for every element on form creation or when a new element is added to a form. This shouldn't change what gets rendered in the form itself because we also check the form_elements.label at that point, but this change means the default label will be populated in the element config form on the form builder.
    • We now set the relationship component to relationBlock when creating a relationship element during initial form creation and when adding a new relationship form element to a form. Previously we were only setting that once the form element was edited.
    • The hover state on the delete/restore buttons on a form element is back!
    • Keyboard interactions work now! Click or use tab to focus an element, then press space to pick it up. Move it up and down with the arrow keys and press space to drop.
    • The "Cancel" button in the side panel component config form won't delete unsaved elements any more unless it's clicked immediately after they've been added
    • The "Save" button on the side panel won't be disabled immediately after adding a new element, so you can use that to exit the panel. Previously the only way was to make an edit or click the x because the cancel button removes the element.

My next PR will remove the order column from form_elements, and add the same dragging/reranking functionality to the pub_values inputs on form fill pages

Test Plan

  • Visit any form
  • Reorder the elements, add some elements, remove some elements
  • Save the form
  • Make sure the order stays the same after you save
  • Visit the live form
  • Make sure the order matches the form builder

@kalilsn kalilsn changed the title Add ranks to form elements Use mudder to rank form elements and related pub values Feb 25, 2025
@kalilsn kalilsn marked this pull request as ready for review February 27, 2025 00:01
@github-actions github-actions bot had a problem deploying to gh-654103159-pr-996 March 4, 2025 20:20 Error
@github-actions github-actions bot had a problem deploying to gh-654103159-pr-996 March 4, 2025 20:36 Error
@github-actions github-actions bot temporarily deployed to gh-654103159-pr-996 March 5, 2025 00:00 Inactive
3mcd and others added 4 commits March 4, 2025 19:41
* chore: set log level=debug

* chore: try removing healthcheck

* chore: add healthcheck back

* chore: attach false maybe

* chore: really not sure

* chore: try removing minio-init

* chore: empty test commit
@github-actions github-actions bot temporarily deployed to gh-654103159-pr-996 March 5, 2025 01:48 Inactive
@tefkah
Copy link
Member

tefkah commented Mar 5, 2025

  • The "Cancel" button in the side panel component config form won't delete unsaved elements any more unless it's clicked immediately after they've been added
  • The "Save" button on the side panel won't be disabled immediately after adding a new element, so you can use that to exit the panel. Previously the only way was to make an edit or click the x because the cancel button removes the element.

you are a hero

Copy link
Member

@tefkah tefkah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good and works well! i made a few suggestions on improving keyboard accessibility and styles, which i think would be good to get in, and a very minor one about code organization which is not necssary. i don't think i tested every possible permutation, but it seems very stable, great work!

@@ -63,22 +67,24 @@ export const FormElement = ({ element, index, isEditing, isDisabled }: FormEleme
removeElement(index);
}}
>
<Trash size={24} className="text-neutral-400 hover:text-red-500" />
<Trash size={24} className="!pointer-events-auto text-neutral-400 hover:text-red-500" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, no, i just applied the latest updates from shadcn there, not sure why they chose to do that.

but you could solve this problem slightly differently, by adding [&_svg]:hover:text-red-500 to the button above. this is slightly nicer i think, as the trash icon turns red if you hover the button, not just the icon

imagine the tip of this arrow being the tip of the hand pointer (can't capture my mouse grrr)
image

disabled={isDisabled || element.deleted}
variant="ghost"
className="invisible p-2 group-hover:visible"
className="invisible p-2 group-hover:visible group-focus:visible"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could fix it by giving things explicit tabIndices, using opacity instead of visibility, and using focus within.

some of these might be redundant, but ill leave comments which should give a setup like so

Screen.Recording.2025-03-05.at.13.33.14.mov

disabled={isDisabled || element.deleted}
variant="ghost"
className="invisible p-2 group-hover:visible"
className="invisible p-2 group-hover:visible group-focus:visible"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so great you made keyboard accessibility work!

it is a bit... strange, it seems that after reordering the tabindex does not per se change (which is understandable, i think?). ends being a strange experience reordering a bunch of items

Screen.Recording.2025-03-05.at.13.20.23.mov

name: string,
slug: string,
communityId: CommunitiesId,
isDefault: boolean,
trx = db
) => {
const ranks = mudder.base62.mudder(pubType.fields.length + 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hahaha wow that would be kind of sick.

i think for inserts this is no problem at all

@3mcd 3mcd temporarily deployed to gh-654103159-pr-996 March 5, 2025 20:18 Inactive
@3mcd 3mcd temporarily deployed to gh-654103159-pr-996 March 5, 2025 20:31 Inactive
@3mcd 3mcd temporarily deployed to gh-654103159-pr-996 March 5, 2025 21:44 Inactive
@3mcd 3mcd had a problem deploying to gh-654103159-pr-996 March 5, 2025 21:52 Error
@kalilsn
Copy link
Member Author

kalilsn commented Mar 5, 2025

Ok this is finally fixed up. There were some more complications with the form builder keyboard accessibility (that were affecting tests) and I also realized it was possible to do the pub value sorting entirely in the database, so I fixed that. I also had to fix the toHaveValues matcher to sort both the received values and the expected values, plus a few other test changes.

@kalilsn kalilsn merged commit e0ae589 into main Mar 5, 2025
8 checks passed
@kalilsn kalilsn deleted the kalilsn/sorting branch March 5, 2025 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Auto-deploys a preview application
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants